-
Notifications
You must be signed in to change notification settings - Fork 237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stop accidentally cleaning up non-expired sessions #2399
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We store session data in the filesystem using our own session `FileStore` class. Each session is stored as a separate JSON file inside `.tmp/sessions` with an expiry date and the data for session. As part of the `set` method for the FileStore class, we have some logic which tries to clean up expired sessions from the filesystem. This loops through all of the files in the session store directory and checks each one in turn to see if the expiry date in the JSON is in the past. If it is, it calls `this.destroy` passing the `storedSessionId`. This happens asynchronously – we map over the files in the directory to create an array of promises, each of which awaits a call to the filesystem to read the JSON. Unfortunately, `storedSessionId` is scoped to the outer `set` method and is therefore affected by the other promises that are running concurrently. Because of this, when there are expired sessions in the session store, it’s possible for non-expired sessions to be destroyed instead, depending on the value of `storedSessionId` at the time destroy is called. Furthermore, because this leaves the expired session in place, further attempts to set data are likely to continue cleaning up the non-expired sessions. You can observe this (and verify that this fixes the issue) by: 1. Adding logging just before the call to this.destroy on line 23: ``` console.log(`Destroying expired session ${storedSessionId}`) ``` 2. Clearing all session data by deleting `.tmp/session` 3. Generating multiple non-expired sessions by refreshing the Prototype Kit multiple times [^1] 4. Manually updating the expiry date in the JSON for one of the sessions to be in the past [^2] 5. Refreshing the kit and observing that a session with a different ID is destroyed Fix this by correctly scoping the `storedSessionId` variable to the asynchronous function, preventing it being altered by other promises resolving at the same time. Due to the limited support we are currently able to provide for the kit, I have made the smallest possible change I believe fixes the bug and not done any further refactoring. Also, whilst I believe it would be beneficial to have tests that cover this code (including this specific bug) I have not done this as it’s non-trivial and involves mocking the file system. Closes #2393. [1]: This works because the request for the new manifest added in GOV.UK Frontend v5.0 (served at `/plugin-assets/govuk-frontend/dist/govuk/assets/manifest.json` in the kit) is made without credentials (see the note on https://web.dev/articles/add-manifest) which results in an extra session being created (and never reused) every time the page is loaded. I think this also might have made this bug more likely to occur because of the sheer number of session files that get created. [2]: In case it’s useful for testing, this jq command shows the expiry date for all current sessions: ``` jq -r -n 'inputs | "\(input_filename) \(.cookie.expires)"' .tmp/sessions/*.json | sort | head ``` Co-authored-by: Colin Rotherham <colin.rotherham@digital.cabinet-office.gov.uk>
wow nice work! |
colinrotherham
approved these changes
Feb 28, 2024
I think this needs a changelog entry? |
36degrees
added a commit
that referenced
this pull request
Feb 28, 2024
Good catch – raised #2401 |
36degrees
added a commit
that referenced
this pull request
Feb 28, 2024
Add changelog entry for #2399
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We store session data in the filesystem using our own session
FileStore
class. Each session is stored as a separate JSON file inside.tmp/sessions
with an expiry date and the data for session.As part of the
set
method for the FileStore class, we have some logic which tries to clean up expired sessions from the filesystem.This loops through all of the files in the session store directory and checks each one in turn to see if the expiry date in the JSON is in the past. If it is, it calls
this.destroy
passing thestoredSessionId
. This happens asynchronously – we map over the files in the directory to create an array of promises, each of which awaits a call to the filesystem to read the JSON.Unfortunately,
storedSessionId
is scoped to the outerset
method and is therefore affected by the other promises that are running concurrently.Because of this, when there are expired sessions in the session store, it’s possible for non-expired sessions to be destroyed instead, depending on the value of
storedSessionId
at the time destroy is called.Furthermore, because this leaves the expired session in place, further attempts to set data are likely to continue cleaning up the non-expired sessions.
You can observe this (and verify that this fixes the issue) by:
Adding logging just before the call to
this.destroy
on line 23:Clearing all session data by deleting
.tmp/session
Generating multiple non-expired sessions by refreshing the Prototype Kit multiple times 1
Manually updating the expiry date in the JSON for one of the sessions to be in the past 2
Refreshing the kit and observing that a session with a different ID is destroyed
Fix this by correctly scoping the
storedSessionId
variable to the asynchronous function, preventing it being altered by other promises resolving at the same time.Due to the limited support we are currently able to provide for the kit, I have made the smallest possible change I believe fixes the bug and not done any further refactoring.
Also, whilst I believe it would be beneficial to have tests that cover this code (including this specific bug) I have not done this as it’s non-trivial and involves mocking the file system.
Closes #2393.
Footnotes
This works because the request for the new manifest added in GOV.UK Frontend v5.0 (served at
/plugin-assets/govuk-frontend/dist/govuk/assets/manifest.json
in the kit) is made without credentials which results in an extra session being created (and never reused) every time the page is loaded. I think this also might have made this bug more likely to occur because of the sheer number of session files that get created. ↩In case it’s useful for testing, this jq command shows the expiry date for all current sessions:
jq -r -n 'inputs | "\(input_filename) \(.cookie.expires)"' .tmp/sessions/*.json
↩